Thanks for this bug report and PR. I started reviewing this but ended up deciding on a simpler design which doesn't create an immediately-ready pollable, and instead tracks the main tasks readiness internally. I re-used your test in #70 but rewrote the rest of this PR.

Thanks for the review @pchickey,

doesn't create an immediately-ready pollable

I did debate whether to just keep polling until the main task eventually returns Pending without being woken, then only poll for the wasi resources.

But then thought won't that actually stall wasi dependent futures if something is always waking the main task up?

And while writing this, I realized one scenario that is valid and also cause wasi futures to stall. futures_lite::future::yield_now(), and actually the custom poll_fn in the test is essentially the same as yield_now. We usually use yield_now in cpu intensive task to cooperatively let other task execute, but if we don't poll wasi resources during that yield opportunity, no other wasi dependent futures can progress.

let cpu_intensive_task = async {
  let work_units = 100_000_000;
  for i in 0..work_units {
    if i % 100 == 0 {
      futures_lite::future::yield_now().await;
    }
    // run some cpu intensive processing
  }
}

Thanks for this bug report and PR. I just merged #71 which fixes the same underlying bug, but has a different implementation which is more efficient. So, I will close this PR now.

I just merged #71 which fixes the same underlying bug, but has a different implementation which is more efficient.

@pchickey, is the concern of stalling wasi futures not valid? It doesn't look like #71 addresses that at all.

The AsyncPollable's WaitFor future checks readiness by making the appropriate Wasi import function pollable.ready call as part of each Future::poll: https://github.com/yoshuawuyts/wstd/blob/main/src/runtime/reactor.rs#L75 https://github.com/yoshuawuyts/wstd/blob/main/src/runtime/reactor.rs#L193

So, even if we don't use wasi:io/poll.poll to block until a wasi pollable is ready, we are still checking if an external event has made it ready as part of each block_on loop.

So, even if we don't use wasi:io/poll.poll to block until a wasi pollable is ready, we are still checking if an external event has made it ready as part of each block_on loop.

Hmm, it seems that this check is only done once on the initial poll, before registering a waker with the reactor. From my understanding with Rust futures, futures doesn't get polled again when it returns Poll::Pending unless something wakes the future right?

On the same line of logic, it's not guaranteed that an AsyncPollable Future::poll gets called on each iteration of block_on loop isn't it? I'm using async-task to implement a simple spawning mechanism for my ease of use in performing background work, in which the implementation explicitly mentions that futures will not reappear in the queue until it has woken up again.

Method run() polls the task's future once. Then, the Runnable vanishes (from the queue) and only reappears when its Waker wakes the task, thus scheduling it to be run again.

Hmm, it seems that this check is only done once on the initial poll, before registering a waker with the reactor.

In the above code, every call of AsyncPollable::poll contains an unconditional call to [Pollable::ready](https://github.com/WebAssembly/wasi-io/blob/main/wit/poll.wit#L11). The registration of the waker occurs if Pollable::ready returns false.

From my understanding with Rust futures, futures doesn't get polled again when it returns Poll::Pending unless something wakes the future right?

My reading of the rust docs is that this is phrased with "should", and because wasip2 is a different sort of operating system than Unix, it is reasonable to break that rule:

The poll function is not called repeatedly in a tight loop – instead, it should only be called when the future indicates that it is ready to make progress (by calling wake()). If you’re familiar with the poll(2) or select(2) syscalls on Unix it’s worth noting that futures typically do not suffer the same problems of “all wakeups must poll all events”; they are more like epoll(4).

An implementation of poll should strive to return quickly, and should not block. Returning quickly prevents unnecessarily clogging up threads or event loops. If it is known ahead of time that a call to poll may end up taking a while, the work should be offloaded to a thread pool (or something similar) to ensure that poll can return quickly.

The implementation of poll does return quickly, and does not block - Pollable::ready does not block, per the spec.. The implementation of block_on in #71 does not tightly loop unless wake has been called, but Pollable::ready may determine that an AsyncPollable is Ready before making a blocking import function call to poll::poll. This design of pollable and poll is designed specifically to prevent what Rust calls the "all wakeups must poll all events" problem.

My reading of the rust docs is that this is phrased with "should", and because wasip2 is a different sort of operating system than Unix, it is reasonable to break that rule:

I don't think we can break that rule, because some crates (like async-task / futures-concurrency) do expect futures to be only re-polled when their wakers wake, not always.
Below is a failing test with #71

    #[test]
    fn cooperative_concurrency() {
        crate::runtime::block_on(async {
            let cpu_heavy = async move {
                // Simulating a CPU-heavy task that runs for 10 second and yields occasionally
                for _ in 0..100 {
                    std::thread::sleep(std::time::Duration::from_millis(100));
                    futures_lite::future::yield_now().await;
                }
                true
            };
            let timeout = async move {
                crate::time::Timer::after(crate::time::Duration::from_secs(3))
                    .wait()
                    .await;
                false
            };
            let mut future_group = futures_concurrency::future::FutureGroup::<
                Pin<Box<dyn std::future::Future<Output = bool>>>,
            >::new();
            future_group.insert(Box::pin(cpu_heavy));
            future_group.insert(Box::pin(timeout));
            let result = future_group.next().await;
            assert_eq!(result, Some(false), "cpu_heavy task should have timed out");
        });
    }

Looking inside the FutureGroup, we find the implementation do expect individual wakers to be woken first, before it polls that particular future again.

The implementation of poll does return quickly, and does not block - Pollable::ready does not block, per the spec.

You are right, I was totally unaware of Pollable::ready function, do you think a good fix for this is to have a branch depending if the main task is awake at the end of the block_on loop, that all pollables are iterated for their readiness via Pollable::ready rather than poll::poll with an immediate?